Add ssh tunnel support for jdbc connectors, issue #312 #5438

airbyte-jenny · 2021-08-16T20:09:24Z

What

Add ssh tunnel support to Java JDBC connectors.

How

Uses apache mina sshd client to connect through ssh, set up a tunnel, and port forward the database ports to where the connector can use them. Tunnel is initiated at a high level of abstraction and closed when it's no longer needed by that particular workflow.

The spec.json configuration required is injected during json deserialization, adding the ssh tunnel config to both the source and the destination for jdbc databases. That means that this feature becomes available to all jdbc sources and destinations without providing connector-specific config screen changes. The user of course does have to fill in specific port forward values per actual connection they create.

Recommended reading order

SSHTunnel.java
AbstractJdbcSource.java
AbstractJdbcDestination.java
Any integration tests for Postgres

Relates to #312

…ort forwarding.

jrhizor

Is tunneling for discovering the catalog for jdbc connectors also going to be supported as part of this PR?

I'd like to take another look once the closing pieces are finished.

jrhizor · 2021-08-19T21:25:13Z

airbyte-integrations/bases/base-java/build.gradle

@@ -5,6 +5,12 @@ plugins {

 dependencies {
    implementation 'commons-cli:commons-cli:1.4'
+    implementation group: 'org.apache.sshd', name: 'sshd-mina', version: '2.7.0'


Suggested change

implementation group: 'org.apache.sshd', name: 'sshd-mina', version: '2.7.0'

implementation 'org.apache.sshd:sshd-mina:2.7.0'

nit: we generally use this format

jrhizor · 2021-08-19T21:26:46Z

airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/BaseConnector.java

+  }
+
+  /**
+   * Extension point for child classes to add things to the spec json tree before it's deserialized


Suggested change

* Extension point for child classes to add things to the spec json tree before it's deserialized

* Extension point for child classes to modify the spec json tree before it's deserialized

jrhizor · 2021-08-19T21:27:00Z

airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/BaseConnector.java

+   * @param root
+   * @return root
+   */
+  public JsonNode addToSpec(JsonNode root) throws IOException {


Suggested change

public JsonNode addToSpec(JsonNode root) throws IOException {

public JsonNode transformSpec(JsonNode root) throws IOException {

Would it makes sense to not expose this method in BaseConnector and instead just doing it on AbstractJdbcDestination and AbstractJdbcSource? I know it means that we will have to override spec() in those respective classes, so it probably a tiny bit more code change. On the plus side though, I think this transformation is going to be pretty specific to our JDBC sources and destinations, so exposing this transform just in those abstractions as opposed to all of our other connectors keeps the base a bit simpler.

jrhizor · 2021-08-19T21:27:14Z

airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/BaseConnector.java

@@ -42,7 +44,18 @@
  public ConnectorSpecification spec() throws Exception {
    // return a JsonSchema representation of the spec for the integration.
    final String resourceString = MoreResources.readResource("spec.json");
-    return Jsons.deserialize(resourceString, ConnectorSpecification.class);
+    return Jsons.object(addToSpec(Jsons.deserialize(resourceString)), ConnectorSpecification.class);


Suggested change

return Jsons.object(addToSpec(Jsons.deserialize(resourceString)), ConnectorSpecification.class);

return Jsons.object(transformSpec(Jsons.deserialize(resourceString)), ConnectorSpecification.class);

jrhizor · 2021-08-19T21:30:21Z

airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/base/SSHTunnel.java

+                   String remoteDatabasePort,
+                   String tunnelDatabasePort) {
+    if (method == null) {
+      this.method = "NO_TUNNEL";


should us a constant for NO_TUNNEL since it's used in a couple places

jrhizor · 2021-08-19T21:37:51Z

airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/base/SSHTunnel.java

+    this.tunnelDatabasePort = tunnelDatabasePort;
+  }
+
+  public static SSHTunnel getInstance(JsonNode config) {


Reading through the code is a bit confusing for the case where the SSHTunnel doesn't have a valid configuration.

Maybe it'd be easier to read if this was Optional<SSHTunnel> getInstance(...)? Then the calls to it could be done with tunnel.ifPresent(...) which might read a bit better to show that this is an optional feature.

can we instead explicitly validate which case we're in (no tunnel/ssh key/pw auth) and do null checks/validation based on that?

jrhizor · 2021-08-19T21:42:08Z

airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/base/SSHTunnel.java

+  /**
+   * Closes a tunnel if one was open, and otherwise doesn't do anything (safe to run).
+   */
+  public void closeTunnel() {


This could actually be close() and the class could be AutoClosable so we could do this with a try-with-resources block instead of a try-finally block.

sherifnada

code changes LGTM - should tests go in a separate PR or this one?

sherifnada · 2021-08-23T14:50:47Z

airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/base/SSHTunnel.java

+    this.tunnelDatabasePort = tunnelDatabasePort;
+  }
+
+  public static SSHTunnel getInstance(JsonNode config) {


can we instead explicitly validate which case we're in (no tunnel/ssh key/pw auth) and do null checks/validation based on that?

sherifnada · 2021-08-23T14:51:28Z

airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/base/SSHTunnel.java

+ * Encapsulates the connection configuration for an ssh tunnel port forward through a proxy/bastion
+ * host plus the remote host and remote port to forward to a specified local port.
+ */
+public class SSHTunnel {


does it make sense to extend autocloseable so we can use a try-with-resources?

sherifnada · 2021-08-23T15:01:36Z

airbyte-integrations/connectors/destination-jdbc/src/main/resources/ssh-tunnel-spec.json

+        "tunnel_method": {
+          "description": "No ssh tunnel needed to connect to database",
+          "type": "string",
+          "enum": ["NO_TUNNEL"],


can we use const: "NO_TUNNEL" instead of enum/default like described here? enum/default shows two dropdowns to the user which is confusing.

Separately it might be better for the UI to name all these options without undesrcores e.g No Tunnel, SSH Key Auth, SSH Password Auth etc.. since it's more user friendly

cgardens

only doing comment because i think we are still waiting on integration tests.

cgardens · 2021-08-24T21:09:46Z

airbyte-integrations/connectors/destination-jdbc/build.gradle

@@ -11,6 +11,7 @@ application {
 dependencies {
    implementation 'com.google.cloud:google-cloud-storage:1.113.16'
    implementation 'com.google.auth:google-auth-library-oauth2-http:0.25.5'
+    implementation group: 'org.apache.sshd', name: 'sshd-mina', version: '2.7.0'


Suggested change

implementation group: 'org.apache.sshd', name: 'sshd-mina', version: '2.7.0'

implementation 'org.apache.sshd:sshd-mina:2.7.0'

cgardens · 2021-08-24T21:17:34Z

airbyte-integrations/connectors/source-relational-db/src/main/resources/ssh-tunnel-spec.json

@@ -0,0 +1,167 @@
+{


is this file the same as the one in airbyte-integrations/connectors/destinations-relational-db/src/main/resources/ssh-tunnel-spec.json? Is there any way we can DRY it out?

cgardens · 2021-08-24T21:23:42Z

airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/BaseConnector.java

+   * @param root
+   * @return root
+   */
+  public JsonNode addToSpec(JsonNode root) throws IOException {


Would it makes sense to not expose this method in BaseConnector and instead just doing it on AbstractJdbcDestination and AbstractJdbcSource? I know it means that we will have to override spec() in those respective classes, so it probably a tiny bit more code change. On the plus side though, I think this transformation is going to be pretty specific to our JDBC sources and destinations, so exposing this transform just in those abstractions as opposed to all of our other connectors keeps the base a bit simpler.

cgardens · 2021-08-24T21:30:23Z

airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/base/SSHTunnel.java

+    if (getHost() == null) {
+      throw new RuntimeException("SSH Tunnel host is null - verify configuration before starting tunnel!");
+    }


Suggested change

if (getHost() == null) {

throw new RuntimeException("SSH Tunnel host is null - verify configuration before starting tunnel!");

}

Preconditions.notNull(getHost(), "SSH Tunnel host is null - verify configuration before starting tunnel!")

we tend to use Preconditions for validations like this. 1. theoretically takes fewer lines of code (though that's not always true) 2. gives you a more specific error message (in this case NPE)

cgardens · 2021-08-24T21:31:55Z

airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/base/SSHTunnel.java

+        Integer.parseInt(
+            getTunnelSshPort().trim()))


Suggested change

Integer.parseInt(

getTunnelSshPort().trim()))

Integer.parseInt(getTunnelSshPort().trim()))

cgardens · 2021-08-24T21:34:34Z

airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/base/SSHTunnel.java

+  public String getMethod() {
+    return method;
+  }
+
+  public String getHost() {
+    return host;
+  }
+
+  public String getTunnelSshPort() {
+    return tunnelSshPort;
+  }
+
+  public String getUser() {
+    return user;
+  }
+
+  private String getSSHKey() {
+    return sshkey;
+  }
+
+  private String getPassword() {
+    return password;
+  }
+
+  public String getRemoteDatabaseHost() {
+    return remoteDatabaseHost;
+  }
+
+  public String getRemoteDatabasePort() {
+    return remoteDatabasePort;
+  }
+
+  public String getTunnelDatabasePort() {
+    return tunnelDatabasePort;
+  }


what's the benefit of exposing all of these getters publicly? should they at least be private? or since these variables are only used internally, can we just reference the fields directly?

…stination

… auth

…s fail yet.

airbyte-jenny · 2021-08-26T20:33:04Z

Existing comments on the PR are probably mostly still relevant; I haven't had a chance to address them. Work is being paused due to timelines for a different feature. This one encountered a lot of complexity around the Source's read() method hierarchy, which is making testing more complex.

What still needs to be done includes identifying the root cause of the test failures for read (either no tunnel starts when needed, or tunnel tries to start twice and port-conflicts itself), get test case for postgres passing, and then go back and address the getSpec() method used for all jdbc connectors in tests, because they're assuming they don't have the ssh tunnel method spec.json nodes in there (but those are added at runtime, and thus the test must accommodate that). Right now that has to be done with duplicated code, because we also haven't addressed the structural issues with test class inheritance that would improve DRY for that.

Basically there are a bunch of structural issues that should be addressed by refactors, but I don't have the time right now to do them. These are notes for when I come back later to try to figure it out further.

To be clear: I expect that destination tests and source tests for non-postgres jdbc connectors are currently failing on testGetSpec() because the static json file they're comparing against doesn't have ssh tunnel configuration in it. I believe source changes and destination changes could be separated into different PRs, but we still have to address the testGetSpec() issue even for destinations.

cgardens · 2021-08-27T00:01:02Z

...airbyte/integrations/destination/postgres/PostgresDestinationSSHTunnelKeyAcceptanceTest.java

+   * but for now that API spec doesn't support that, so we'll do it here.
+   * @throws Exception
+   */
+  void dropUniqueSchema() throws Exception {


why do we need to use and ssh tunnel to interact with the database for setting up test fixtures? aren't we just making life harder for ourselves?

ah. because the test database we are using is only accessible via ssh.

cgardens · 2021-08-27T00:01:48Z

@airbyte-jenny how were you testing this locally? was there a special docker image for postgres that you were using that had the bastion onboard? or were you just running on test infra?

CLAassistant · 2021-09-27T17:31:36Z

All committers have signed the CLA.

sherifnada · 2021-09-29T07:53:15Z

closing since this was done in other PRs

airbyte-jenny added 14 commits August 16, 2021 15:06

Created specification for ssh tunnel configuration

e0e857c

Read ssh tunnel config for destination postgres

2268889

Added apache mina sshd for ssh client tunnel support

c0db5bb

Integrate ssh key format parsing. Fix up json config reading.

dfd8660

Undid log format experiment

2246ffc

Cleanup

cf989a7

Got ssh key format conversion working

71632a5

Got ssh key format conversion working

72372d4

Readability improvement

b03b09d

Comments

eaff495

Cleanup on bouncycastle jars

a4f6e4d

Fix permissions for test user to allow connect

034d23e

Made ssh tunnel work correctly. No more jar conflict. Correct local p…

74604ec

…ort forwarding.

Fixed for ssh tunneling

9326192

github-actions bot added the area/connectors Connector related issues label Aug 16, 2021

airbyte-jenny added 3 commits August 16, 2021 17:52

Refactor ssh tunnel to be easily reused

00d872b

Made spec.json injection of ssh tunnel happen in a shared, abstract way

ad2b9cf

Whitespace formatting

d79e6cf

airbyte-jenny requested review from jrhizor, sherifnada and Phlair August 19, 2021 16:06

airbyte-jenny changed the title ~~Airbyte jenny/connector ssh tunnels issue 312~~ Add ssh tunnel support for jdbc connectors, issue #312 Aug 19, 2021

jrhizor reviewed Aug 19, 2021

View reviewed changes

sherifnada reviewed Aug 23, 2021

View reviewed changes

WIP: Integration tests

5308038

cgardens reviewed Aug 24, 2021

View reviewed changes

airbyte-jenny added 3 commits August 25, 2021 10:40

Make integration tests work for ssh tunnel connection for postgres de…

52feb8b

…stination

Variant of the test using password auth for ssh tunnel instead of key…

5a4b979

… auth

Work in progress on ssh tunnel in source connectors. Integration test…

ff7238f

…s fail yet.

cgardens reviewed Aug 27, 2021

View reviewed changes

sherifnada closed this Sep 29, 2021

swyxio deleted the airbyte-jenny/connector_ssh_tunnels_issue_312 branch October 12, 2022 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ssh tunnel support for jdbc connectors, issue #312 #5438

Add ssh tunnel support for jdbc connectors, issue #312 #5438

airbyte-jenny commented Aug 16, 2021 •

edited

Loading

jrhizor left a comment

jrhizor Aug 19, 2021

jrhizor Aug 19, 2021

jrhizor Aug 19, 2021

cgardens Aug 24, 2021

jrhizor Aug 19, 2021

jrhizor Aug 19, 2021

jrhizor Aug 19, 2021

sherifnada Aug 23, 2021

jrhizor Aug 19, 2021

sherifnada left a comment

sherifnada Aug 23, 2021

sherifnada Aug 23, 2021

sherifnada Aug 23, 2021

cgardens left a comment

cgardens Aug 24, 2021

cgardens Aug 24, 2021

cgardens Aug 24, 2021

cgardens Aug 24, 2021

cgardens Aug 24, 2021

cgardens Aug 24, 2021

airbyte-jenny commented Aug 26, 2021 •

edited

Loading

cgardens Aug 27, 2021

cgardens Aug 27, 2021

cgardens commented Aug 27, 2021 •

edited

Loading

CLAassistant commented Sep 27, 2021 •

edited

Loading

sherifnada commented Sep 29, 2021

	implementation group: 'org.apache.sshd', name: 'sshd-mina', version: '2.7.0'
	implementation 'org.apache.sshd:sshd-mina:2.7.0'

	* Extension point for child classes to add things to the spec json tree before it's deserialized
	* Extension point for child classes to modify the spec json tree before it's deserialized

	public JsonNode addToSpec(JsonNode root) throws IOException {
	public JsonNode transformSpec(JsonNode root) throws IOException {

	return Jsons.object(addToSpec(Jsons.deserialize(resourceString)), ConnectorSpecification.class);
	return Jsons.object(transformSpec(Jsons.deserialize(resourceString)), ConnectorSpecification.class);

	Integer.parseInt(
	getTunnelSshPort().trim()))
	Integer.parseInt(getTunnelSshPort().trim()))

Add ssh tunnel support for jdbc connectors, issue #312 #5438

Add ssh tunnel support for jdbc connectors, issue #312 #5438

Conversation

airbyte-jenny commented Aug 16, 2021 • edited Loading

What

How

Recommended reading order

jrhizor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherifnada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgardens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

airbyte-jenny commented Aug 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgardens commented Aug 27, 2021 • edited Loading

CLAassistant commented Sep 27, 2021 • edited Loading

sherifnada commented Sep 29, 2021

airbyte-jenny commented Aug 16, 2021 •

edited

Loading

airbyte-jenny commented Aug 26, 2021 •

edited

Loading

cgardens commented Aug 27, 2021 •

edited

Loading

CLAassistant commented Sep 27, 2021 •

edited

Loading